Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Get docusaurus building #1434

Open
wants to merge 3 commits into
base: fdc3-for-web-impl
Choose a base branch
from

Conversation

julianna-ciq
Copy link
Contributor

No description provided.

@julianna-ciq julianna-ciq requested a review from a team as a code owner November 13, 2024 15:03
@robmoffat
Copy link
Member

I think moving docs/ into website/ might be a step too far... but I'll leave @kriswest to think about that

Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see an issue in the schema handling: docs pages when generated should point to the 'next' version of schemas. Then the versioning scripts should generate copies of those pages, copies of the schemas and update links from one to the other so we get frozen versions of both associated with each other.

See all the package.json scripts that start with version - I think the paths are all still correct as I think those all operate off the website/static folder paths... However, the copy-schemas and copy-appd scripts definitely now have incorrect paths in them. This page: https://fdc3.finos.org/schemas/next/app-directory.html may be broken in your version... Try deleting this file: https://github.com/InteropIO/FDC3/blob/monorepo-docusaurus/website/static/schemas/next/appd.schema.json
then build and if the file doesn't get replaced its broken.

For testing context schemas try the schema link any page in the next version - but watch out for URL hardcoded to their production values, you'll need to sub-in the local or preview origin. Same goes for API schemas in the new 'specs' pages.

It'd also be helpful to have a quick word in teh PR description on why moving docs into the website folder helps (I do know its the normal pattern for a docusaurus site). The license covering said docs (the CSL) remains in the root directory. 'The Standard' is essentially the docs and schema files - the schema files now reside in /packages/fdc3-schema and /packages/fdc3-context and appear to be under the Apache-2.0 license (as thats whats in their package.json files).

@robmoffat I don't actually know the licensing implications here. The schemas contain information that is not in the docs and need to be a formally under the CSL, as do the docs. If this is problematic, it is so in both cases - perhaps worse in the schema packages as they explicitly state a different license...

website/schema2Markdown.js Outdated Show resolved Hide resolved
website/docs/context/ref/Action.md Outdated Show resolved Hide resolved
@julianna-ciq
Copy link
Contributor Author

I think moving docs/ into website/ might be a step too far... but I'll leave @kriswest to think about that

Docusaurus is pretty opinionated on where the docs files should live. There were a bunch of build errors that just disappeared once I moved the files into the folder docusaurus likes them to be in. If you want the docs folder to stay separate, I assume that you can, but there's going to be a lot of juggling on someone's part.

@kriswest
Copy link
Contributor

Docusaurus is pretty opinionated on where the docs files should live. There were a bunch of build errors that just disappeared once I moved the files into the folder docusaurus likes them to be in. If you want the docs folder to stay separate, I assume that you can, but there's going to be a lot of juggling on someone's part.

Hmm, thats odd as AFAIK we aren't getting build errors in the pre-refactor version with docs up a level and the docusaurus config pointing at them. Got any details on the errors (out of interest)? Or any details on the juggling needed if we don't do it?

@kriswest
Copy link
Contributor

Oh and we've got this PR hanging out there as part of the Citi Hackathon, which is trying to upgrade existing docs to work in Docusaurus 3: #1418

I'd guess we'll have to make some similar changes for anything introduced since then. I can ask them to rebase on main as the fdc3 for web docs just got merged in.

@julianna-ciq
Copy link
Contributor Author

I've updated the copy/paste scripts to move the schemas from the correct place.

@kriswest
Copy link
Contributor

kriswest commented Nov 13, 2024

I've updated the copy/paste scripts to move the schemas from the correct place.

Thanks @julianna-ciq, don't forget to run schema2markdown again, it should drop a bunch of the context docs pages from the diff (currently 217 files) by eliminating the change to the schema URL.

I'm seeing some additional differences in the context docs pages that are reverting recent changes, e.g. instrumentList losing its id and name properties:
https://github.com/finos/FDC3/pull/1434/files?show-deleted-files=true&show-viewed-files=true&file-filters%5B%5D=#diff-1a9127f623217fd78ece8885a417aca8246835fb70184b8630e7128d1921cf3d

Those should vanish as well, thats probably a result of generating from the 2.1 schemas instead of next 🤞

@kriswest
Copy link
Contributor

@robmoffat and @julianna-ciq
I've further corrected the issues in docs generation from context schemas and run the generation. I'm satisfied that is all working correctly. I've also satisfied myself that there are no significant licensing issues around moving either the schemas or docs folders (but then I'm no IP lawyer ;-) ).

I only see one issue with moving the docs folder into /website and that is the docusaurus v3 update PR #1418. @julianna-ciq has done the move correctly so that should merge with this just fine. We just need to get #1418 updated from main, with any additional fixes for recently merged context applied and then merged and we should be good to go.

I would say its safe to merge this into fdc3-for-web-impl while we get #1418 updated and merged.

@robmoffat
Copy link
Member

can you re-review? It won't let me merge it until you're not "requesting changes"

@kriswest kriswest self-requested a review November 20, 2024 12:34
Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants